Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix a request that hangs around after an unexpected response #209

Closed
wants to merge 1 commit into from

Conversation

aktau
Copy link

@aktau aktau commented Jun 26, 2013

In my setup, sometimes my node websocket server goes away. At that point nginx tells any further clients that there is a Bad Gateway (502). Of course the WebSocket client then errors out, but it forgets to abort the request. For websocket connections, nginx must be configured with a keep-alive value, so the connection can take a long time to time out. This ties up unnecesary resources.

@3rd-Eden
Copy link
Member

@aktau Could you add a test for this?

@aktau
Copy link
Author

aktau commented Jun 26, 2013

I'm not really sure how to do that, before this project I had never even touched node.js and even though I knew js I'm not really familiar with the testing frameworks.

I'm assuming you would have to make a server that:

  1. keeps a request alive
  2. returns a non-recogized status code like 502

Then you would have to check that the request does not hang. I really have no idea how to this since you can't get at the request (it's rightfully not exposed) and there is no real way to see if it hangs afaik because the only way I noticed it is by seeing that my node process did not exit properly.

That said, this is a one-line change that can't (imho) really do much wrong. The request provoked an error, it needs to end anyway, nothing productive can still be done on that request (as far as websockets is confirmed). So it doesn't do any harm to close it.

@davedoesdev
Copy link
Contributor

+1 for fixing this issue.
It can also be caused by sending an error status in response to an upgrade request.
I'll see if I can work out a test.

@davedoesdev
Copy link
Contributor

#271

I fixed it slightly differently in that I expose the request and response in the error object.
This:

  • Keeps existing behaviour if nothing else is done.
  • Gives a chance to call request.abort() if required.
  • Gives a chance to read the data from the response (or other header values). This can be very useful when you want to return extra error information!

@aktau @3rd-Eden Is this a good approach do you think?

@lpinca
Copy link
Member

lpinca commented Oct 18, 2016

Closing this as it has been addressed by #271.
Thank you!

@lpinca lpinca closed this Oct 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants